-
Notifications
You must be signed in to change notification settings - Fork 91
Fix: Lock can be acquired after lease duration if the current owner has been terminated unexpectedly when using skipBlockingWait=true #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…the time values could be in different jvms - nonoTime will no longer work
| */ | ||
| public long millisecondTime() { | ||
| return System.nanoTime() / 1000000; | ||
| return System.currentTimeMillis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear this PR doesn't account for clock skew nor monotonicity and introduces correctness bugs around those concepts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't updating this pre-existing method outside the scope of this CR? Unless I am missing something after reading https://stackoverflow.com/questions/351565/system-currenttimemillis-vs-system-nanotime this looks like an enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating this pre-existing method is required for the idea behind this PR, which is to use wall clock time to determine if a client can acquire the lock.
currentTimeMillis is wall clock time (can be converted into a time like 2023-06-13 8:37 PM)
nanoTime is an entirely random number that is only useful to measure elapsed time
I would propose #88 as a more nuanced PR as it:
- does not affect blocking clients by changing this method which is critical for correctness
- surfaces clock skew to the end user as a problem they need to consider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thank you for explaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should abandon this proposed change for #88 after it is revised.
|
I am facing the same issue with 1.2.0 version, is this going to be merged or any other solution to this? A lock is stale in my DDB and no other instance is able to acquire the lock. I am setting withShouldSkipBlockingWait. |
Issue #, if available: #44
Description of changes:
The change in this PR after resolving few conflicts
#79
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.